feat(buzz-agent): gate LLM tool calls on session/request_permission - #5712
Open
wpfleger96 wants to merge 4 commits into
Open
feat(buzz-agent): gate LLM tool calls on session/request_permission#5712wpfleger96 wants to merge 4 commits into
wpfleger96 wants to merge 4 commits into
Conversation
Every LLM-issued MCP tool call now asks the client to authorize it before executing. buzz-agent always asks; the client applies BUZZ_ACP_PERMISSION_POLICY. The agent never reads policy, matching the layering of the other ACP harnesses. A crate-local PermissionBroker owns the full correlation lifecycle: a process-wide admission semaphore (BUZZ_AGENT_MAX_PENDING_PERMISSIONS, default 32) acquired before any correlation entry is inserted, a monotonic id allocator, an abort-safe PendingPermission lease whose Drop synchronously removes the entry and releases the slot, claim-before-wake delivery for at-most-once resolution, and a single absolute deadline (BUZZ_AGENT_PERMISSION_TIMEOUT_SECS, default 330s) shared by admission and response wait. Cancellation races inside the wait, never depending on the outer abort drain. The request builder is version-aware, keyed on the protocol version negotiated at initialize and stored for the connection lifetime: v2 nests the tool call under subject, v1 uses the legacy top-level shape. Authorization is fail-closed: execute IFF outcome is "selected" and the selected optionId equals the offered allow option; every other shape denies with a synthetic tool error and the turn continues. Argument-shape validation is hoisted ahead of the ask so a malformed call is rejected locally without prompting. load_skill and _Stop/ _PostCompact lifecycle hooks are exempt — they are not model-issued. Tests: a subprocess + fake-MCP boundary suite proves no call reaches MCP before approval, exact-allow reaches it once, and reject/cancelled/ error/malformed/unknown-outcome/wrong-option/stale-id all fail closed, plus crossed-parallel isolation and the two exemptions; broker unit tests prove timeout, abort, and multi-session admission invariants with an injectable deadline. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…laim-before-wake ordering, reject noncanonical ids Address three review findings on the permission broker's ask surface. An undeliverable ask is now terminal. wire::send_checked surfaces the mpsc send failure that occurs exactly when the writer task has exited on a closed/broken stdout; request_permission fails closed immediately on that error (dropping the lease removes the entry and releases the permit synchronously) instead of leaving a resident waiter to time out. async_main now selects on both the reader and the writer JoinHandle: writer death cancels every session and closes the reader lifecycle rather than reading input while asks wait out their deadline for a reply that can never be written. Claim-before-wake ordering is now mutation-sensitive. Production enforces it structurally — the oneshot sender is consumed by remove, so a send-before-remove mutant cannot compile without swapping the channel type. A test-only wake observer fires synchronously in the waiter's response arm and asserts the entry is already absent from pending at the wake; a faithful wake-before-claim mutant makes it observe false and the test goes red while the behavioral delivery tests stay green. parse_id now requires an exact canonical round-trip, so noncanonical aliases (perm-01, perm-+0, perm-00) that u64::parse would accept are rejected as foreign ids rather than resolving live asks. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
… by the deadline Close two wire-closure terminality gaps in the permission broker's ask surface. Writer flush failure is now connection-fatal. A blocking stdout can report Ok from write_all when it only schedules the underlying write and surface the real error at flush; the writer previously discarded that flush error and kept waiting on its receiver, so a genuinely dead stdout left the writer alive, the async_main writer-death arm never fired, and an accepted ask stayed resident until its deadline. write_frames (extracted, generic over its AsyncWrite sink for tests) now returns on either write_all or flush error, dropping the receiver so the connection supervisor cancels every session. Permission enqueue is now the third phase governed by the single absolute deadline. request_permission previously bare-awaited send_checked after registering the entry; a full-but-live channel makes that await wait for capacity, racing neither cancellation nor the deadline, so a stalled writer could hold the ask and its global permit past the advertised deadline and session/cancel could not resolve it. The send now runs in the same biased select as admission and response: cancel wins Cancelled, the shared deadline wins the timeout deny, send error stays the wire-closed deny. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
wesbillman
requested changes
Aug 14, 2026
wesbillman
left a comment
wesbillman
left a comment
Collaborator
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Requesting changes for one fail-open malformed-response path in the authorization boundary.
wire::classify classifies every object with an id and no string-valued method as Inbound::Response, then forwards result whenever it exists (crates/buzz-agent/src/wire.rs:101-127). It does not require the JSON-RPC response shape to contain exactly one of result or error, and a present non-string method is indistinguishable from an absent method. Consequently either malformed frame below reaches PermissionBroker::evaluate and authorizes the tool:
{"jsonrpc":"2.0","id":"perm-0","result":{"outcome":{"outcome":"selected","optionId":"allow_once"}},"error":{"code":-32600,"message":"invalid"}}
{"jsonrpc":"2.0","id":"perm-0","method":7,"result":{"outcome":{"outcome":"selected","optionId":"allow_once"}}}That contradicts the documented fail-closed invariant for malformed responses (permission.rs:67-74, 316-325). This is an authorization gate, so malformed protocol input must not be normalized into a valid approval. Please validate response structure before forwarding it: no method member at all, exactly one of result/error, and an error response always delivered as denial (or represent invalid separately and deny the matching waiter). Add regression coverage for both ambiguous result+error and non-string method frames proving the MCP tool is not invoked.
I otherwise found the call-path gate, correlation lease, cancellation/deadline handling, and intended hook/load-skill exemptions sound at exact head a3256c068b17b21c9c0ea3e0129f952665363855.
wire::classify normalized malformed JSON-RPC responses into well-formed allow results before the fail-closed broker could see them. A present non-string `method` collapsed to "no method" via `as_str`, and a frame with both `result` and `error` forwarded `result` unconditionally — so a `selected`/`allow_once` payload in either shape was laundered into an approval upstream of every authorization check. classify now forwards `result` only for a structurally valid response: no `method` member and exactly one of `result`/`error`. Any other shape normalizes to Null, which the broker denies. The adversarial-response tests attacked the result payload; these attack the frame structure. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
buzz-agent now authorizes every LLM-issued MCP tool call through
session/request_permissionbefore it executes, instead of running tools unconditionally. The agent always asks; the client appliesBUZZ_ACP_PERMISSION_POLICYand answers. buzz-agent never reads the policy — this keeps the policy decision on the client side, matching the layering of the other ACP harnesses, and avoids duplicating policy logic that would drift.Broker
A crate-local
PermissionBroker(crates/buzz-agent/src/permission.rs), owned byAppfor the connection lifetime, owns the full request-correlation lifecycle:Semaphore(BUZZ_AGENT_MAX_PENDING_PERMISSIONS, default 32, validated>= 1) is acquired before any correlation entry is inserted. This is the security bound: the per-turn tool semaphore is constructed fresh per turn andmax_sessionsis unbounded by default, so neither bounds simultaneously-outstanding asks process-wide.PendingPermissionlease that owns the admission permit and the correlation id. ItsDropsynchronously removes the still-pending entry and releases the slot, covering task abort/panic that bypasses the normalrun_prompttail.deliverclaims (removes) the entry before waking the waiter, so each id resolves once and a later leaseDropis a no-op. Unknown/late ids are logged and dropped; only ids the broker minted (perm-<n>) are recognized.BUZZ_AGENT_PERMISSION_TIMEOUT_SECS, default 330s, validated>= 1), so a saturated call cannot outlive one timeout window even when a stalled writer blocks the enqueue. Cancellation races inside every wait — resolution never depends on the outer abort drain. A writer that dies mid-connection (stdout closed, or a blocking write that only surfaces its error at flush) is connection-fatal: it cancels all sessions, which resolves any ask waiting on a reply that can never be written.Wire
request_permission_params(crates/buzz-agent/src/wire.rs) is version-aware, keyed on the protocol version negotiated atinitializeand stored onAppfor the connection lifetime (never derived from a later mutable session field). v2 nests the tool call undersubject: {type: "tool_call", toolCall}with top-leveltitle/options; v1 uses the legacy top-leveltoolCall. No hybrid shape. Both offered options (allow_once,reject_once) carryoptionId == kind, so the client'skind-based selection and this side'soptionId-based predicate agree without a lookup table.Gate
In each spawned tool task (
crates/buzz-agent/src/agent.rs) the sequence is: acquire per-turn permit → argument-shape validation → broker admission + request + wait → cancellation recheck →emit_in_progress→mcp.call. Argument-shape validation is hoisted out ofmcp.rs::do_callintovalidate_arg_shapeso a malformed non-object argument is rejected locally without prompting for a call that could never execute.Authorization is fail-closed, stated once in
evaluate: execute IFFoutcome.outcome == "selected"and the selectedoptionIdequals the offered allow option. Every other shape (reject, cancelled, JSON-RPC error, malformed, unknown outcome, wrong/unknown option, timeout, wire-channel closure) denies with a synthetic tool error, and the turn continues.Scope
Only LLM-issued MCP calls are gated. The built-in
load_skilltool andcall_hookslifecycle calls (_Stop,_PostCompact) are exempt — they are not model-issued.readOnlyHintis never treated as a security boundary.First cut ships
allow_once/reject_onceonly; session-scoped grants are deliberately out of scope.Related issue
Part of #4938. This PR and #5106 jointly implement the feature: #5106 is the client-side policy engine and permission cards; this PR is buzz-agent's asking side (
session/request_permission). Neither closes #4938 alone.